-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(browser): Add graphqlClientIntegration
#13783
base: develop
Are you sure you want to change the base?
feat(browser): Add graphqlClientIntegration
#13783
Conversation
Added support for graphql query with `xhr` with tests. Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
@@ -357,6 +357,8 @@ export function xhrCallback( | |||
return undefined; | |||
} | |||
|
|||
const requestBody = JSON.parse(sentryXhrData.body as string); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is dangerous, any such operations we have to try-catch as bodies may not be JSON!
@@ -374,6 +376,7 @@ export function xhrCallback( | |||
'server.address': host, | |||
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.browser', | |||
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client', | |||
body: requestBody, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h: We should not do this - this will attach the request bodies to all spans, always, which is a) large, b) potentially PII sensitive 😬
Instead of doing this in on('spanStart')
, I think we'll need a hook that provides the request or body in some way. I am thinking of a new hook like:
client.on('outgoingRequestSpanStart', (span: Span, { body }: { body: unknown }) => {
// ...
});
and emit this hook in this file after the span was started 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted. I implemented that hook and attached the req payload only to graphql spans.
Added test for fetch graphql. Created new utility functions and added tests. Updated `instrumentFetch` to collect fetch request payload. Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
const handlerData: HandlerDataFetch = { | ||
args, | ||
fetchData: { | ||
method, | ||
url, | ||
body, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO collecting fetch req payload shouldn't be a problem as long as only the graphql requests are sampled. Because xhr instrumentation already collects the payload.
Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
|
…r graphql requests Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
const requestBody = JSON.parse(payload); | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
const isGraphQLRequest = !!requestBody['query']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not be the most accurate and safest way to detect a graphql request?
client.on('spanStart', span => { | ||
client.emit('outgoingRequestSpanStart', span); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was maybe a bit confusingly explained by me - I would not do this, so I'd remove these lines here. I'll leave a comment further below where/how I would suggest to do this!
client.on('spanStart', span => { | |
client.emit('outgoingRequestSpanStart', span); | |
}); |
@@ -374,6 +377,7 @@ export function xhrCallback( | |||
'server.address': host, | |||
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.browser', | |||
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client', | |||
body: graphqlRequest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding this here, after the span was started, in this file do:
client.emit('outgoingRequestSpanStart', span, { body });
So the hook receives the body as second argument, and then in the integration we can use the body and add it to the span or use it otherwise. This way, this is not added to all spans, as we do not want to generally capture this for every span.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get what you mean now! Will implement
… has started Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
Just wanted to inform about the failing tests:
|
Resolves #13399
Todo:
Supportfetch
spans and testsSupport shorthand graphql queries (i.e., without a name)Moved to laterEnhance breadcrumb datayarn lint
) & (yarn test
).